[PM-27575] Add support for loading Mailer templates from disk #6520
Conversation
|
Claude finished @Hinton's task —— View job Code Review CompleteSummary of Changes Since Last ReviewLatest commit (9e608b4) contains only formatting changes (reordering using statements). No functional changes since previous review. Addressing Previous Comment ThreadResponding to Hinton's question about Finding 1 from the previous review: Previous Finding 1 is incorrect. The null reference scenario described cannot occur. Here's why: AnalysisThe
Therefore, when the code reaches line 96, Your response is correct - any exception would be caught by the catch block, but more importantly, the specific null reference scenario cannot actually occur with the current logic. New Findings❌ Finding 1: Typo in test method nameLine 71 of HandlebarMailRendererTests.cs: Method name
|
| @@ -54,19 +67,50 @@ private async Task<HandlebarsTemplate<object, object>> CompileTemplateInternalAs | |||
| return handlebars.Compile(source); | |||
| } | |||
|
|
|||
| private static async Task<string> ReadSourceAsync(Assembly assembly, string template) | |||
There was a problem hiding this comment.
We had to remove static from most functions because reading from disk relies on non static data.
|
New Issues (4)Checkmarx found the following issues in this Pull Request
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #6520 +/- ##
===========================================
+ Coverage 13.30% 52.50% +39.20%
===========================================
Files 1137 1910 +773
Lines 49787 84792 +35005
Branches 3879 7574 +3695
===========================================
+ Hits 6625 44524 +37899
+ Misses 43040 38542 -4498
- Partials 122 1726 +1604 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
# Conflicts: # test/Core.Test/Platform/Mailer/HandlebarMailRendererTests.cs # test/Core.Test/Platform/Mailer/MailerTest.cs
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
|
||
| try | ||
| { | ||
| var diskPath = Path.GetFullPath(Path.Combine(_globalSettings.MailTemplateDirectory, template)); |
There was a problem hiding this comment.
❌ Finding 1: Null reference exception when MailTemplateDirectory is null
Details
When SelfHosted is true but MailTemplateDirectory is not explicitly configured, GlobalSettings.BuildDirectory returns null (line 143 in GlobalSettings.cs). This causes Path.Combine and Path.GetFullPath to throw an ArgumentNullException.
The BuildDirectory method only returns a non-null value when:
- An explicit value is set, OR
SelfHostedis true (returns/etc/bitwarden/mail-templates)
However, the check on line 89 only validates SelfHosted, not whether MailTemplateDirectory is null.
Suggested fix:
if (!_globalSettings.SelfHosted || string.IsNullOrWhiteSpace(_globalSettings.MailTemplateDirectory))
{
return null;
}There was a problem hiding this comment.
That's caught by the catch right?
| var baseDirectory = Path.GetFullPath(_globalSettings.MailTemplateDirectory); | ||
|
|
||
| // Ensure the resolved path is within the configured directory | ||
| if (!diskPath.StartsWith(baseDirectory + Path.DirectorySeparatorChar, StringComparison.OrdinalIgnoreCase) && |
There was a problem hiding this comment.
The path validation uses StringComparison.OrdinalIgnoreCase, but this only affects string comparison logic, not actual filesystem behavior. On Linux (case-sensitive), an attacker could potentially bypass this check by exploiting case differences if symbolic links or actual directory names differ in case from the configured path.
Consider using Path.GetFullPath on both sides after resolving any symbolic links, or add explicit checks for symbolic link traversal.
|
|
||
| if (File.Exists(diskPath)) | ||
| { | ||
| var fileContents = await File.ReadAllTextAsync(diskPath); |
There was a problem hiding this comment.
🎨 Finding 3: Variable name fileContents can be simplified to content
The variable assignment on line 109 followed by immediate return on line 110 can be simplified to:
return await File.ReadAllTextAsync(diskPath);This reduces unnecessary variable allocation and improves readability.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-27575
📔 Objective
Adds support for overloading mail templates from disk.
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes